Skip to content

update node info processors to include unschedulable nodes #8520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

update node info processors to include unschedulable nodes #8520

wants to merge 2 commits into from

Conversation

@elmiko
Copy link
Contributor

@elmiko elmiko commented Sep 10, 2025

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR adds a new lister for ready unschedulable nodes, it also connects that lister to a new parameter in the node info processors Process function. This change enables the autoscaler to use unschedulable, but otherwise ready, nodes as a last resort when creating node templates for scheduling simulation.

Which issue(s) this PR fixes:

Fixes #8380

Special notes for your reviewer:

I'm not sure if this is the best way to solve this problem, but i am proposing this for further discussion and design.

Does this PR introduce a user-facing change?

Node groups where all the nodes are ready but unschedulable will be processed as potential candidates for scaling when simulating cluster scheduling.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/needs-area cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 10, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: elmiko
Once this PR has been reviewed and has the lgtm label, please assign aleksandra-malinowska for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/needs-area labels Sep 10, 2025
@elmiko
Copy link
Contributor Author

elmiko commented Sep 10, 2025

i'm working on adding more unit tests for this behavior, but i wanted to share this solution so we could start talking about it.

@elmiko elmiko force-pushed the unschedulable-nodes-fix branch from a0ebb28 to 3270172 Compare October 2, 2025 20:50
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 2, 2025
@elmiko
Copy link
Contributor Author

elmiko commented Oct 2, 2025

i've rewritten this patch to use all nodes as the secondary value instead of using a new list of ready unschedulable nodes.

@elmiko elmiko changed the title WIP update to include unschedulable nodes update node info processors to include unschedulable nodes Oct 2, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2025
@elmiko
Copy link
Contributor Author

elmiko commented Oct 2, 2025

i need to do a little more testing on this locally, but i think this is fine for review.

elmiko
elmiko commented Oct 3, 2025
View reviewed changes
cluster-autoscaler/processors/nodeinfosprovider/mixed_nodeinfos_processor.go Outdated
// Last resort - unready/unschedulable nodes.
for _, node := range nodes {
// we want to check not only the ready nodes, but also ready unschedulable nodes.
for _, node := range append(nodes, allNodes...) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure that this is appropriate to append these. theoretically the allNodes should already contain nodes. i'm going to test this out using just allNodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

due to filtering that happens in obtainNodeLists, we need to combine both lists of nodes here.

@elmiko elmiko force-pushed the unschedulable-nodes-fix branch from 3270172 to cb2649a Compare October 3, 2025 16:37
@elmiko
Copy link
Contributor Author

elmiko commented Oct 3, 2025

i updated the argument names in the Process function to make the source of the nodes more clear. i also changed the mixed node info processor to not double count the nodes for the unschedulable/unready detection clause.

@elmiko
Copy link
Contributor Author

elmiko commented Oct 3, 2025

it seems like the update to the mixed node processor needs a little more investigation.

@elmiko elmiko force-pushed the unschedulable-nodes-fix branch from cb2649a to fd53c0b Compare October 3, 2025 16:59
@elmiko
Copy link
Contributor Author

elmiko commented Oct 3, 2025

it looks like we need both the readyNodes and allNodes lists due to the filtering that happens in the core.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2025
@elmiko elmiko force-pushed the unschedulable-nodes-fix branch from fd53c0b to 906a939 Compare October 8, 2025 18:44
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2025
@elmiko
Copy link
Contributor Author

elmiko commented Oct 8, 2025

rebased

@elmiko
Copy link
Contributor Author

elmiko commented Oct 14, 2025

@jackfrancis @towca any chance at a review here?

@elmiko
Copy link
Contributor Author

elmiko commented Oct 16, 2025

In any case, IMO the most readable change would be to:

  • Start passing allNodes instead of readyNodes to TemplateNodeInfoProvider.Process() without changing the signature. This is what the interface definition suggests anyway.
  • At the beginning of MixedTemplateNodeInfoProvider.Process(), group the passed allNodes into good and bad candidates utilizing isNodeGoodTemplateCandidate(). Then iterate over the good ones in the first loop, and over the bad ones in the last loop.

i can put together a patch like this and give it some tests.

@elmiko elmiko force-pushed the unschedulable-nodes-fix branch from 906a939 to 5244a8f Compare October 22, 2025 15:30
@elmiko
Copy link
Contributor Author

elmiko commented Oct 22, 2025

rebased and updated with the requested changes.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 22, 2025
towca
towca reviewed Oct 22, 2025
View reviewed changes
@elmiko
pass allNodes to node info provider Process
d0adaa7 
This change passes all the nodes to the mixed node info provider
processor that is called from `RunOnce`. The change is to allow
unschedulable and unready nodes to be processed as bad canidates during
the node info template generation.

The Process function has been updated to separate nodes into good and
bad candidates to make the filtering match the original intent.
@elmiko elmiko force-pushed the unschedulable-nodes-fix branch from 5244a8f to 42518f1 Compare October 24, 2025 20:01
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 24, 2025
@elmiko
Copy link
Contributor Author

elmiko commented Oct 24, 2025

refactored to put the unschedulable flag clearing behind a flag. i'm not totally happy with this solution as it feels a little sneaky to add the boolean value to the TaintConfig struct. but, it is cleaner this way.

@elmiko elmiko force-pushed the unschedulable-nodes-fix branch from 42518f1 to c1f4a88 Compare October 24, 2025 20:03
@elmiko
add --ignore-node-unschedulable flag
57994b4 
This change introduces a flag which will instruct the CA to ignore a
node's `.spec.unschedulable` field when creating node template for
considering which node group to scale.
@elmiko elmiko force-pushed the unschedulable-nodes-fix branch from c1f4a88 to 57994b4 Compare October 24, 2025 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jackfrancis jackfrancis jackfrancis left review comments

@towca towca towca left review comments

@vadasambar vadasambar Awaiting requested review from vadasambar

@x13n x13n Awaiting requested review from x13n

Assignees

No one assigned

Labels

area/cluster-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CA potential for skipped node template info when a node group contains only non-ready nodes

4 participants

@elmiko @k8s-ci-robot @jackfrancis @towca